Repository navigation
feat(contexts): give applications a menu with its own API - #3135
rubenvdlinde wants to merge 2 commits into
Conversation
9847aa9 to
5825db2
Compare
5825db2 to
9e81a78
Compare
7e236cc to
c7a6e95
Compare
| return null; | ||
| } | ||
|
|
||
| $table = $schema->createTable(self::TABLE); |
There was a problem hiding this comment.
Done. The proposal is on #3100 and blizzz has answered it. This PR is rebuilt on that proposal: one table, tables_contexts_menu_item, created by a single migration.
| $table->addColumn('icon', Types::STRING, ['notnull' => false, 'length' => 64]); | ||
| $table->addColumn('description', Types::TEXT, ['notnull' => false]); | ||
| $table->addColumn('target_type', Types::STRING, ['notnull' => true, 'length' => 16]); | ||
| $table->addColumn('target_id', Types::INTEGER, ['notnull' => false]); |
There was a problem hiding this comment.
Could menu items use node_rel_id, like widgets? Then removing a table can also remove its menu item
There was a problem hiding this comment.
Yes. A table or view item now points at its node_rel_id. When the table or view leaves the application, its items are removed in the same transaction as its widget sources.
| } | ||
|
|
||
| return $this->formatResultRows($r, $userId); | ||
| return $this->formatResultRows($r, $userId, $this->menuItemMapper->findByContextIds([$contextId])); |
There was a problem hiding this comment.
Permission checks use findById(), so each check now runs this menu query for no reason. Maybe should be conditional
There was a problem hiding this comment.
Thanks, that query is gone. The menu is no longer loaded with the application. It has its own endpoint, GET /api/2/menu?contextId=, so findById() and the permission checks don't touch it at all.
| // Deleting the application first takes the write lock before anything is read, so on | ||
| // SQLite a concurrent write fails this transaction at once and the retry runs it again. | ||
| $this->atomicRetry(function () use ($context): void { | ||
| $this->contextMapper->delete($context); | ||
| $this->shareService->deleteAllForContext($context); | ||
| $this->menuItemMapper->deleteAllByContextId($context->getId()); |
There was a problem hiding this comment.
should this be in a different PR?
There was a problem hiding this comment.
Yes. That change came out of this PR. All that's left in ContextService is what the menu needs: a new application gets one item for its start page, and deleting an application or one of its tables or views removes the items that open it.
c7a6e95 to
8574da7
Compare
9af2352 to
7d89fa5
Compare
An application gets a menu of items. An item opens at most one thing: a
page of the application, a table or view of the application by its node
relation, or an http or https link. An item that opens nothing is a
group; groups are top-level and only groups hold items, so a menu is one
level deep. An item without a title shows the title of what it opens.
An item has no technical name of its own. A menu holds at most 200
items.
The menu has its own endpoints on api/2: GET /menu?contextId= returns the
menu as a flat list in display order, POST /menu adds an item to the
end, PATCH and DELETE /menu/{itemId} change or delete one item, and
PUT /menu/order saves the order and grouping of the whole menu in one
transaction. Reading needs read access to the application and changes
need manage access, checked once through the item's application.
Outsiders get a 404, readers who try a change a 403 and a target outside
the application a 400. Create is limited to 20 a minute per user, a
change or an order to 60.
Of the three target columns (page, node relation, link) at most one is
set, which the service enforces. Creating or changing an item checks its
target in the transaction that saves it. Empty groups stay in the menu;
readers without manage access don't see them.
A new application gets one item, without a title, for its start page.
An item goes when its target goes, in the same transaction: with its
page, when its table or view leaves the application or is deleted, and
with the application. The items of a deleted group move to the top
level. An item whose target is gone anyway is left out when the menu is
read. Applications that exist today have no items and open their start
page.
Co-authored-by: remko48 <remko@conduction.nl>
Assisted-by: ClaudeCode:claude-opus-5-5
Signed-off-by: Ruben van der Linde <ruben@conduction.nl>
Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ruben van der Linde <ruben@conduction.nl>
7d89fa5 to
aa0c7e3
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
Part of the stack on #3133. Implements the menu proposal on #3100: the model and the API. Export and import follow in #3138. This replaces the earlier version of this pull request, together with #3136 and #3137.
Size: 3322 lines: 909 code, 414 tests, 1991 generated OpenAPI and 8 version.
Agreed on #3100
page_id,node_rel_id,url), of which at most one is set. The service enforces it; the rule is in the entity docblock.contextId, like the other application routes, notcontextas in the proposal.What changes
{pageId}), a table or view of the application by its node relation ({nodeRelId}), or an http or https link ({url}). An item that opens nothing is a group. Groups are top-level and only groups hold items, so a menu is one level deep.title(as stored) anddisplayTitle.nodes, a scheme import, or because the table or view is deleted, also with its owner), and with the application. The items of a deleted group move to the top level.#[ApiRoute]:GET /api/2/menu?contextId={id}returns the menu as a flat list in display order, withparentIdPOST /api/2/menuadds an item to the end:contextId,title,icon,description,targetPATCH /api/2/menu/{itemId}changes an item; a target replaces the previous one, a group that holds items can't get oneDELETE /api/2/menu/{itemId}deletes an itemPUT /api/2/menu/ordertakescontextIdanditems: [{id, parentId}]in display order. It lists every item exactly once, a parent is a top-level group listed before its items, and it saves all of it or nothing ("changed in between, reload"). It returns the whole menu.#[RequirePermission]covers the endpoints withcontextId; the item endpoints check through the item's application.#[UserRateLimit]: create 20 a minute, change and order 60 a minute.How it was tested
On Nextcloud 34 with PostgreSQL:
MenuServiceTestagainst the database: the start page item of a new application, title fallback for every kind of target, invalid items (data provider), at most one target, empty groups hidden from readers, the 200-item cap, PATCH semantics, a group with items can't get a target, order and grouping, deleting a group, invalid orders that save nothing (data provider), cleanup with the page, the node and the application, a dangling item left out, and only managers change the menu (data provider)MenuControllerTest: arguments passed on and every error mapped to its status (data providers)composer cs:check, psalm and rector, all clean🖼️ Screenshots
Backend only, no UI changes.
🏁 Checklist
/backport to stableX.X🤖 AI (if applicable)